CCIP-4160 Token Transfer Tests #15278

0xsuryansh · 2024-11-18T05:19:15Z

CCIP Integration test covering the following scenarios

Send token to EOA
Send token to contract
Send 2 tokens to receiver
Send N tokens to contract

github-actions · 2024-11-18T05:35:53Z

AER Report: CI Core ran successfully ✅

aer_workflow , commit

AER Report: Operator UI CI ran successfully ✅

aer_workflow , commit

mateusz-sekara

Good job, some minor comments

mateusz-sekara · 2024-11-19T15:23:17Z

integration-tests/smoke/ccip_test.go

+	return srcToken, dstToken
+}
+
+func mintAndApprove(t *testing.T, e ccdeploy.DeployedEnv, state ccdeploy.CCIPOnChainState, token *burn_mint_erc677.BurnMintERC677, chainSel uint64, amount *big.Int) {


I think we have a very similar helper for the USDC, it would be great to dedup these

chainlink/integration-tests/smoke/ccip_usdc_test.go

Line 254 in 5a68674

func mintAndAllow(

mateusz-sekara · 2024-11-19T15:24:09Z

integration-tests/smoke/ccip_test.go

+				// Fetch initial balance
+				if _, exists := initialBalances[dstToken.Address()]; !exists {
+					balance, err := dstToken.BalanceOf(nil, scenario.receiver)
+					require.NoError(t, err)


Similar story already for USDC, if worth the effort / makes sense we can dedup this code

mateusz-sekara · 2024-11-19T15:25:17Z

integration-tests/smoke/ccip_test.go

+				},
+			},
+			receiver: e.Chains[tenv.FeedChainSel].DeployerKey.From,
+			data:     []byte(""),


I suggest making data as nil or empty slice to make it more explicit

mateusz-sekara · 2024-11-19T15:27:59Z

integration-tests/smoke/ccip_test.go

-	_, err = e.Chains[tenv.HomeChainSel].Confirm(tx)
-	require.NoError(t, err)
+	// Deploy and approve tokens.
+	srcToken1, dstToken1 := deployAndApproveTokens(t, tenv, state, "Token1")


I think it was part of the ticket to also deploy some tokens in self serve manner. Right now, we deploy everything using the DeployerKey. We want to also verify it token transfers work properly in the self-serve deployed pool. Probably you need to create a new EOA and deploy token contracts from that

integration-tests/smoke/ccip_test.go

makramkd · 2024-11-20T12:35:41Z

integration-tests/smoke/ccip_test.go

+		dstChain     uint64
+		tokenAmounts []router.ClientEVMTokenAmount
+		receiver     common.Address
+		data         []byte


If data is always nil for this test, do we need this in the scenario?

For programmable token transfer tests data will not be nil right ?

integration-tests/smoke/ccip_test.go

# Conflicts: # integration-tests/smoke/ccip_test.go

# Conflicts: # integration-tests/smoke/ccip/ccip_test.go

…IP-4160_Implement_token_transfer_tests # Conflicts: # deployment/ccip/changeset/test_helpers.go

… and pools

…t_token_transfer_tests

dimkouv

Mostly minor comments, approving. Feel free to resolve and re-request review if you agree with any.

dimkouv · 2024-11-28T16:17:55Z

deployment/ccip/changeset/test_helpers.go

+		for _, token := range tokens {
+			twoCoins := new(big.Int).Mul(big.NewInt(1e18), big.NewInt(2))
+
+			owner, ok := owners[chain]


this could be moved in the outer loop.

dimkouv · 2024-11-28T16:19:08Z

deployment/ccip/changeset/test_helpers.go

+	expectedSeqNum := make(map[SourceDestPair]uint64)
+	expectedSeqNumExec := make(map[SourceDestPair][]uint64)
+
+	latesthdr, err := env.Chains[destChain].Client.HeaderByNumber(testcontext.Get(t), nil)


maybe better to pass ctx to this func?

dimkouv · 2024-11-28T16:20:03Z

deployment/ccip/changeset/test_helpers.go

+	require.NoError(t, err)
+
+	require.Eventually(t, func() bool {
+		actualBalance, err := tokenContract.BalanceOf(&bind.CallOpts{Context: tests.Context(t)}, receiver)


maybe pass ctx to this func? to avoid getting a new one everyime.

dimkouv · 2024-11-28T16:20:19Z

deployment/ccip/changeset/test_helpers.go

+	tokenContract, err := burn_mint_erc677.NewBurnMintERC677(token, chain.Client)
+	require.NoError(t, err)
+
+	balance, err := tokenContract.BalanceOf(&bind.CallOpts{Context: tests.Context(t)}, receiver)


similar for ctx

dimkouv · 2024-11-28T16:21:13Z

integration-tests/smoke/ccip/ccip_token_transfer_test.go

+	inMemoryEnv := false
+
+	// use this if you are testing locally in memory
+	// tenv := changeset.NewMemoryEnvironmentWithJobsAndContracts(t, lggr, 2, 4, config)


would be better if we can do this using some env var. So we can easily switch in CI or run without touching code.

dimkouv · 2024-11-28T16:22:09Z

integration-tests/smoke/ccip/ccip_token_transfer_test.go

+		selfServeDestTokenPoolDeployer,
+		state,
+		e.ExistingAddresses,
+		"SELF_SERVE_TOKEN",


dimkouv · 2024-11-28T16:22:36Z

integration-tests/smoke/ccip/ccip_token_transfer_test.go

+
+	tinyOneCoin := new(big.Int).SetUint64(1)
+
+	// Test scenarios are defined here


comment probably not necessary, it's obvious

dimkouv · 2024-11-28T16:24:54Z

integration-tests/smoke/ccip/ccip_token_transfer_test.go

+			tokenAmounts: []router.ClientEVMTokenAmount{
+				{
+					Token:  srcToken.Address(),
+					Amount: tinyOneCoin,


Name is a bit confusing for me tinyOneCoin maybe just one and oneE18, fiveE18 etc ?

dimkouv · 2024-11-28T16:25:46Z

integration-tests/smoke/ccip/ccip_token_transfer_test.go

+			},
+			receiver: state.Chains[sourceChain].Receiver.Address(),
+			expectedTokenBalances: map[common.Address]*big.Int{
+				selfServeSrcToken.Address(): new(big.Int).SetUint64(2),


can set two to a variable to follow same pattern with tinyOneCoin

dimkouv · 2024-11-28T16:26:29Z

integration-tests/smoke/ccip/ccip_token_transfer_test.go

+	require.NoError(t, err)
+
+	// Simulated backend sets chainID to 1337 always
+	chainID := big.NewInt(1337)


i believe it's similar for the commented out environment setup.

b-gopalswami · 2024-11-28T16:30:05Z

integration-tests/smoke/ccip/ccip_test.go

@@ -75,134 +73,3 @@ func TestInitialDeployOnLocal(t *testing.T) {

 	// TODO: Apply the proposal.
 }
-
-func TestTokenTransfer(t *testing.T) {


Nit: As we moved out this test, the parallel count here can be reduced to one. https://github.com/smartcontractkit/chainlink/pull/15278/files#diff-c97a5a0f8e46926381c7f53223fb048419a40e9c9b8e243b3f49e15bc2e00d9eL945

b-gopalswami · 2024-11-28T16:44:55Z

integration-tests/smoke/ccip/ccip_token_transfer_test.go

+	for _, scenario := range scenarios {
+		t.Run(scenario.name, func(t *testing.T) {
+			initialBalances := map[common.Address]*big.Int{}
+			for token := range scenario.expectedTokenBalances {
+				initialBalance := changeset.GetTokenBalance(t, token, scenario.receiver, e.Chains[scenario.dstChain])
+				initialBalances[token] = initialBalance
+			}
+
+			changeset.TransferAndWaitForSuccess(
+				t,
+				e,
+				state,
+				scenario.srcChain,
+				scenario.dstChain,
+				scenario.tokenAmounts,
+				scenario.receiver,
+				scenario.data,
+				scenario.expectedExecutionState,
+			)
+
+			for token, balance := range scenario.expectedTokenBalances {
+				expected := new(big.Int).Add(initialBalances[token], balance)
+				changeset.WaitForTheTokenBalance(t, token, scenario.receiver, e.Chains[scenario.dstChain], expected)
+			}
+		})
+	}


This approach is quite sequential. I would recommend splitting this test into multiple smaller tests and enabling parallel execution.

A few options we could consider include:

Using multiple wallet addresses.

Breaking the test into separate segments (though this will require repeating the setup), and assigning a dedicated runner for each segment. This would reduce the overall execution time, as the message processing wait time can be parallelized across the different tests. With this approach, we could potentially achieve at least a 3x speedup.

0xsuryansh changed the title ~~CCIP-4160 test refactoring~~ CCIP-4160 Token Transfer Tests Nov 18, 2024

CCIP-4160 test refactoring

3bfac63

0xsuryansh force-pushed the CCIP-4160_Implement_token_transfer_tests branch from 99c3452 to 3bfac63 Compare November 19, 2024 11:33

0xsuryansh marked this pull request as ready for review November 19, 2024 11:35

0xsuryansh requested review from a team as code owners November 19, 2024 11:35

0xsuryansh requested review from winder, rstout, asoliman92, makramkd, dimkouv, mateusz-sekara and 0xAustinWang November 19, 2024 11:35

0xsuryansh added 3 commits November 19, 2024 18:21

resolving imports and using local dev env

4617621

comments fix

24dd4f6

comments fix

8bae382

mateusz-sekara reviewed Nov 19, 2024

View reviewed changes

makramkd reviewed Nov 20, 2024

View reviewed changes

0xsuryansh and others added 11 commits November 20, 2024 20:52

Merge branch 'develop' into CCIP-4160_Implement_token_transfer_tests

b4a4a98

# Conflicts: # integration-tests/smoke/ccip_test.go

Merge branch 'develop' into CCIP-4160_Implement_token_transfer_tests

25c1b15

# Conflicts: # integration-tests/smoke/ccip_test.go

test update with common methods

94a2527

import fix for lint

9be1966

Merge branch 'develop' into CCIP-4160_Implement_token_transfer_tests

18f5f2e

# Conflicts: # integration-tests/smoke/ccip/ccip_test.go

Merge branch 'develop' into CCIP-4160_Implement_token_transfer_tests

7f8fd25

# Conflicts: # integration-tests/smoke/ccip/ccip_test.go

refactor: updated test with new setup

3d969d6

fix: lint (imports)

e2182dd

Merge branch 'develop' into CCIP-4160_Implement_token_transfer_tests

8273eac

Yolo

1bc99b3

Merge branch 'develop' into CCIP-4160_Implement_token_transfer_tests

e1a80af

# Conflicts: # integration-tests/smoke/ccip/ccip_test.go

0xsuryansh added 2 commits November 27, 2024 18:07

Merge remote-tracking branch 'origin/ms/self-serve-addr-test' into CC…

691300c

…IP-4160_Implement_token_transfer_tests # Conflicts: # deployment/ccip/changeset/test_helpers.go

feat: self serve pool test + ability to add custom deployer for token…

8f4d4c5

… and pools

0xsuryansh requested a review from a team as a code owner November 27, 2024 16:15

0xsuryansh and others added 8 commits November 27, 2024 22:42

fix : Use LocalDevEnvironment

36df2f6

temp: trying out with AdditionalSimulatedPvtKeys

faca5a5

Merge branch 'develop' into CCIP-4160_Implement_token_transfer_tests

211e668

fix: lints and unused function

c53e872

Merge branch 'develop' into CCIP-4160_Implement_token_transfer_tests

1a03b58

fix: lints and unused function

9037d57

fix: imports for lint

f829199

Move tt tests to a separate file

eee7c93

mateusz-sekara requested review from a team as code owners November 28, 2024 12:32

mateusz-sekara added 7 commits November 28, 2024 13:41

Move tt tests to a separate file

947b68d

Fix

eedb8fa

Fix

8308df9

Merge remote-tracking branch 'origin/develop' into CCIP-4160_Implemen…

6b726d7

…t_token_transfer_tests

Fix

20a017e

Fix

97c74df

Fix

7f3fb4f

mateusz-sekara approved these changes Nov 28, 2024

View reviewed changes

mateusz-sekara requested a review from makramkd November 28, 2024 14:26

mateusz-sekara enabled auto-merge November 28, 2024 14:52

dimkouv approved these changes Nov 28, 2024

View reviewed changes

b-gopalswami reviewed Nov 28, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CCIP-4160 Token Transfer Tests #15278

CCIP-4160 Token Transfer Tests #15278

0xsuryansh commented Nov 18, 2024 •

edited

Loading

github-actions bot commented Nov 18, 2024 •

edited

Loading

mateusz-sekara left a comment

mateusz-sekara Nov 19, 2024

mateusz-sekara Nov 19, 2024

mateusz-sekara Nov 19, 2024

mateusz-sekara Nov 19, 2024

makramkd Nov 20, 2024

0xsuryansh Nov 20, 2024

dimkouv left a comment

dimkouv Nov 28, 2024

dimkouv Nov 28, 2024

dimkouv Nov 28, 2024

dimkouv Nov 28, 2024

dimkouv Nov 28, 2024

dimkouv Nov 28, 2024

dimkouv Nov 28, 2024

dimkouv Nov 28, 2024

dimkouv Nov 28, 2024

dimkouv Nov 28, 2024

b-gopalswami Nov 28, 2024

b-gopalswami Nov 28, 2024


		tinyOneCoin := new(big.Int).SetUint64(1)

		// Test scenarios are defined here

CCIP-4160 Token Transfer Tests #15278

Are you sure you want to change the base?

CCIP-4160 Token Transfer Tests #15278

Conversation

0xsuryansh commented Nov 18, 2024 • edited Loading

github-actions bot commented Nov 18, 2024 • edited Loading

AER Report: CI Core ran successfully ✅

AER Report: Operator UI CI ran successfully ✅

mateusz-sekara left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dimkouv left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0xsuryansh commented Nov 18, 2024 •

edited

Loading

github-actions bot commented Nov 18, 2024 •

edited

Loading